Introduce FileStreamStrategy as a first step of FileStream rewrite #47128

adamsitnik · 2021-01-18T18:19:40Z

As discussed offline, we would like to introduce a few dedicated FileStream implementations (imagine one for sync and one for async file operations on Windows for example).

Since this is more or less an implementation of the Strategy Pattern I've called the new abstraction a FileStreamStrategy. For now, there is just a single implementation, so I've named it FileStreamImpl but in the future, you should expect nicer names that described the unique strategy (an example could be IoUringFileStrategy).

The goal of this PR was to check if it's possible to choose the strategy in ctor and what performance impact an introduction of a new abstraction layer would bring.

It's is possible, mostly because of the fact that .NET was Windows-only from the beginning and if we want to have async file operations on Windows, we have to specify a special flag when opening the file. This has shaped FileStream ctor and luckily for us, this information is always present when we are creating a new instance of FileStream

To measure the performance impact I've revisited the existing FileStream benchmarks and added missing ones (see dotnet/performance#1633 for details).

The introduction of a new abstraction layer has a very small footprint. The differences are typically within the 1% range. Since these are IO benchmarks and they are not as stable as CPU benchmarks, I believe that the overhead is within the margin of error. The overhead is small because most of the time is spent in expensive syscalls and one extra virtual method call is not an issue (verified with profiler)

If we agree that this is the right direction, the next steps will be most probably:

separation of Windows and Unix implementation (dedicated PR to keep the diff as small as possibe)
separation of sync and async Windows implementations (dedicated PR to keep the diff as small as possibe)
dividing the User Story into smaller work items and distributing the work amongst the Team (a lot of small PRs)

~~If you are about to review this PR, PTAL at the commits as I've tried to keep them small, and reviewing each of them might be the easiest way to review this PR.~~ Edit: I've looked at the diff and it's not as bad as I expected.

In the details, you can find full benchmark results:

Method	Toolchain	fileSize	userBufferSize	options	Mean	Ratio	Allocated
Read	after	1024	512	None	49.35 us	1.00	4,328 B
Read	before	1024	512	None	49.18 us	1.00	4,288 B

Write	after	1024	512	None	998.73 us	1.02	4,334 B
Write	before	1024	512	None	981.14 us	1.00	4,296 B

ReadAsync	after	1024	512	None	70.32 us	1.02	4,888 B
ReadAsync	before	1024	512	None	69.19 us	1.00	4,848 B

WriteAsync	after	1024	512	None	1,023.21 us	0.99	4,880 B
WriteAsync	before	1024	512	None	1,036.22 us	1.00	4,840 B

ReadAsync	after	1024	512	Asynchronous	70.54 us	1.01	4,752 B
ReadAsync	before	1024	512	Asynchronous	69.87 us	1.00	4,713 B

WriteAsync	after	1024	512	Asynchronous	1,039.03 us	1.00	4,730 B
WriteAsync	before	1024	512	Asynchronous	1,038.29 us	1.00	4,696 B

Read	after	1024	4096	None	48.34 us	1.00	208 B
Read	before	1024	4096	None	48.29 us	1.00	168 B

Write	after	1024	4096	None	192.81 us	0.99	209 B
Write	before	1024	4096	None	195.00 us	1.00	168 B

ReadAsync	after	1024	4096	None	64.61 us	0.98	656 B
ReadAsync	before	1024	4096	None	65.75 us	1.00	616 B

WriteAsync	after	1024	4096	None	185.73 us	0.97	208 B
WriteAsync	before	1024	4096	None	191.24 us	1.00	168 B

ReadAsync	after	1024	4096	Asynchronous	83.25 us	1.00	800 B
ReadAsync	before	1024	4096	Asynchronous	83.19 us	1.00	760 B

WriteAsync	after	1024	4096	Asynchronous	196.46 us	1.01	241 B
WriteAsync	before	1024	4096	Asynchronous	194.08 us	1.00	200 B

OpenClose	after	1024	?	None	40.44 us	1.01	208 B
OpenClose	before	1024	?	None	40.21 us	1.00	168 B

LockUnlock	after	1024	?	None	49.59 us	0.99	208 B
LockUnlock	before	1024	?	None	50.10 us	1.00	168 B

SeekForward	after	1024	?	None	260.89 us	1.00	209 B
SeekForward	before	1024	?	None	262.01 us	1.00	168 B

SeekBackward	after	1024	?	None	1,648.66 us	1.00	208 B
SeekBackward	before	1024	?	None	1,648.17 us	1.00	168 B

ReadByte	after	1024	?	None	56.01 us	1.01	4,328 B
ReadByte	before	1024	?	None	55.53 us	1.00	4,288 B

WriteByte	after	1024	?	None	1,046.60 us	1.00	4,334 B
WriteByte	before	1024	?	None	1,049.12 us	1.00	4,288 B

CopyToFile	after	1024	?	None	1,073.59 us	1.02	8,663 B
CopyToFile	before	1024	?	None	1,057.57 us	1.00	8,577 B

CopyToFileAsync	after	1024	?	None	1,090.13 us	0.99	9,545 B
CopyToFileAsync	before	1024	?	None	1,107.22 us	1.00	9,466 B

OpenClose	after	1024	?	Asynchronous	42.36 us	0.99	240 B
OpenClose	before	1024	?	Asynchronous	42.62 us	1.00	200 B

LockUnlock	after	1024	?	Asynchronous	54.04 us	1.00	240 B
LockUnlock	before	1024	?	Asynchronous	54.03 us	1.00	200 B

SeekForward	after	1024	?	Asynchronous	2,279.64 us	0.99	253 B
SeekForward	before	1024	?	Asynchronous	2,307.75 us	1.00	201 B

SeekBackward	after	1024	?	Asynchronous	4,727.69 us	1.00	247 B
SeekBackward	before	1024	?	Asynchronous	4,720.73 us	1.00	201 B

ReadByte	after	1024	?	Asynchronous	75.84 us	0.99	4,681 B
ReadByte	before	1024	?	Asynchronous	76.40 us	1.00	4,642 B

WriteByte	after	1024	?	Asynchronous	1,106.40 us	1.00	4,726 B
WriteByte	before	1024	?	Asynchronous	1,106.71 us	1.00	4,683 B

CopyToFileAsync	after	1024	?	Asynchronous	1,188.76 us	1.00	9,956 B
CopyToFileAsync	before	1024	?	Asynchronous	1,186.00 us	1.00	9,865 B

Read	after	1048576	512	None	717.18 us	0.99	4,332 B
Read	before	1048576	512	None	725.25 us	1.00	4,292 B

Write	after	1048576	512	None	6,107.45 us	0.99	4,338 B
Write	before	1048576	512	None	6,166.35 us	1.00	4,318 B

ReadAsync	after	1048576	512	None	3,484.60 us	1.01	234,041 B
ReadAsync	before	1048576	512	None	3,464.68 us	1.00	234,001 B

WriteAsync	after	1048576	512	None	10,250.67 us	1.01	234,034 B
WriteAsync	before	1048576	512	None	10,168.80 us	1.00	233,995 B

ReadAsync	after	1048576	512	Asynchronous	3,963.44 us	1.00	41,562 B
ReadAsync	before	1048576	512	Asynchronous	3,973.24 us	1.00	41,558 B

WriteAsync	after	1048576	512	Asynchronous	10,970.55 us	0.98	41,643 B
WriteAsync	before	1048576	512	Asynchronous	11,233.77 us	1.00	41,602 B

Read	after	1048576	4096	None	671.74 us	1.00	212 B
Read	before	1048576	4096	None	672.49 us	1.00	168 B

Write	after	1048576	4096	None	6,189.52 us	1.00	255 B
Write	before	1048576	4096	None	6,195.09 us	1.00	215 B

ReadAsync	after	1048576	4096	None	1,004.08 us	1.02	29,216 B
ReadAsync	before	1048576	4096	None	986.96 us	1.00	29,176 B

WriteAsync	after	1048576	4096	None	6,877.03 us	1.00	29,209 B
WriteAsync	before	1048576	4096	None	6,863.35 us	1.00	29,169 B

ReadAsync	after	1048576	4096	Asynchronous	4,692.88 us	0.95	80,361 B
ReadAsync	before	1048576	4096	Asynchronous	4,930.79 us	1.00	80,321 B

WriteAsync	after	1048576	4096	Asynchronous	18,727.57 us	1.00	80,357 B
WriteAsync	before	1048576	4096	Asynchronous	18,636.56 us	1.00	80,317 B

CopyToFile	after	1048576	?	None	4,995.21 us	0.99	422 B
CopyToFile	before	1048576	?	None	5,055.80 us	1.00	372 B

CopyToFileAsync	after	1048576	?	None	5,271.18 us	1.00	2,885 B
CopyToFileAsync	before	1048576	?	None	5,261.36 us	1.00	2,805 B

CopyToFileAsync	after	1048576	?	Asynchronous	5,795.09 us	1.00	7,999 B
CopyToFileAsync	before	1048576	?	Asynchronous	5,796.33 us	1.00	7,925 B

Read	after	104857600	512	None	88,023.79 us	1.00	4,344 B
Read	before	104857600	512	None	87,632.37 us	1.00	4,304 B

Write	after	104857600	512	None	195,172.14 us	1.08	4,360 B
Write	before	104857600	512	None	184,907.73 us	1.00	4,320 B

ReadAsync	after	104857600	512	None	375,406.06 us	1.00	22,942,328 B
ReadAsync	before	104857600	512	None	375,625.89 us	1.00	22,942,288 B

WriteAsync	after	104857600	512	None	576,623.27 us	1.00	22,942,360 B
WriteAsync	before	104857600	512	None	578,749.96 us	1.00	22,942,320 B

ReadAsync	after	104857600	512	Asynchronous	416,641.01 us	1.00	3,700,840 B
ReadAsync	before	104857600	512	Asynchronous	418,745.91 us	1.00	3,699,016 B

WriteAsync	after	104857600	512	Asynchronous	650,181.89 us	1.00	3,692,000 B
WriteAsync	before	104857600	512	Asynchronous	647,189.04 us	1.00	3,691,200 B

Read	after	104857600	4096	None	82,057.39 us	1.00	224 B
Read	before	104857600	4096	None	81,915.68 us	1.00	184 B

Write	after	104857600	4096	None	190,406.22 us	1.06	240 B
Write	before	104857600	4096	None	183,531.85 us	1.00	232 B

ReadAsync	after	104857600	4096	None	127,204.26 us	0.99	2,867,776 B
ReadAsync	before	104857600	4096	None	128,555.62 us	1.00	2,867,736 B

WriteAsync	after	104857600	4096	None	261,657.17 us	1.05	2,867,800 B
WriteAsync	before	104857600	4096	None	254,019.01 us	1.00	2,867,760 B

ReadAsync	after	104857600	4096	Asynchronous	484,225.08 us	0.97	7,987,752 B
ReadAsync	before	104857600	4096	Asynchronous	501,833.12 us	1.00	7,987,712 B

WriteAsync	after	104857600	4096	Asynchronous	2,056,012.93 us	1.03	7,987,872 B
WriteAsync	before	104857600	4096	Asynchronous	2,010,968.44 us	1.00	7,987,704 B

CopyToFile	after	104857600	?	None	75,856.57 us	1.01	504 B
CopyToFile	before	104857600	?	None	75,283.88 us	1.00	424 B

CopyToFileAsync	after	104857600	?	None	81,167.16 us	0.99	180,380 B
CopyToFileAsync	before	104857600	?	None	82,353.32 us	1.00	180,360 B

CopyToFileAsync	after	104857600	?	Asynchronous	145,859.23 us	1.04	255,704 B
CopyToFileAsync	before	104857600	?	Asynchronous	141,232.08 us	1.00	255,368 B

cc @jeffhandley

…to call, implement these methods

…cellationToken)

…default)

…ncellationToken)

…nToken = default)

…ck, object? state)

…ack, object? state)

adamsitnik · 2021-02-08T14:41:00Z

I wonder why.

In this particular case it's not a CPU, but an IO benchmark so we have more moving targets: OS heuristics for data caching and the hardware itself. This introduces a lot of instability. It looks that the bigger the file size, the more unstable the Write benchmarks become.

When I set the number of iterations to 100, I get values from 147,138.40 us to 804,459.50 us for Write, while for read it's from 82,490.75 us to 91,019.90 us

carlossanlop

Question (as Adam would famously say) for my own education:

If we want to preserve the old FileStream behavior (guarded by a compat flag), then we would directly call the code in WindowsFileStreamStrategy or UnixFileStreamStrategy. And as we add more and more strategies, then we should always keep in mind to leave the original code unmodified. Is this correct?

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamStrategyBase.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

stephentoub · 2021-02-17T22:43:34Z

If we want to preserve the old FileStream behavior (guarded by a compat flag), then we would directly call the code in WindowsFileStreamStrategy or UnixFileStreamStrategy. And as we add more and more strategies, then we should always keep in mind to leave the original code unmodified. Is this correct?

I think we'd effectively move the existing logic into a:

private sealed class LegacyStrategy : FileStreamStrategy
{
    ... // existing code goes here, just modifed to fit with the abstraction appropriately
}

and then in the various constructors:

public FileStream(...)
{
    if (AppCompatSwitchSet)
    {
        _strategy = new LegacyStrategy(...);
    }
    else
    {
        _strategy = ... // the better thing
    }
}

…mStrategy

adamsitnik · 2021-02-22T20:13:39Z

I think we'd effectively move the existing logic into a:

done. I've given up on splitting the Unix and Windows implementations in this PR and decided to just introduce the concept of strategy and moving the existing code into LegacyFileStreamStrategy. This gives us a new abstraction layer, but 0 breaking changes. The breaking changes will be introduced in separate PRs, but to new strategies, not LegacyFileStreamStrategy.

@stephentoub @jozkee @carlossanlop PTAL, the PR is ready for review

stephentoub · 2021-02-23T17:02:14Z

src/libraries/System.Private.CoreLib/src/System/IO/DerivedFileStreamStrategy.cs

+    // for everything else we fall back to the actual strategy (like FileStream does)
+    //
+    // it's crucial to NOT use the "base" keyoword here! everything must be using _fileStream or _strategy
+    internal sealed class DerivedFileStreamStrategy : FileStreamStrategy


I think "Derived" is an odd name here, given that FileStreamStrategy is abstract and any implementation will necessarily be "derived". We often use the term "Delegating" in such cases, e.g. DelegatingHandler, DelegatingStream, etc.

I think "Derived" is an odd name here, given that FileStreamStrategy is abstract and any implementation will necessarily be "derived"

In this particular context, it meant a strategy for a type that derives from FileStream. Does it make more sense or would you like me to rename it to DelegatingFileStreamStrategy ?

So "DerivedFileStreamFileStreamStrategy" :)

I don't have a strong opinion. If you want to stick with DerivedFileStreamStrategy, that's fine.

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

stephentoub · 2021-02-23T17:13:04Z

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

+            if (_strategy.IsClosed)
                throw Error.GetFileNotOpen();


Would it be cheaper if this were in the _strategy.WriteAsync method itself? Seems like we could potentially avoid one additional virtual dispatch?

We could, but here I did this on purpose to keep all the validation logic in FileStream and keep all the strategies free of any such checks (mostly to avoid code duplication).

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs

stephentoub · 2021-02-23T17:31:13Z

src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs

+        ~LegacyFileStreamStrategy()
+        {
+            // it looks like having this finalizer is mandatory,
+            // as we can not guarantee that the Strategy won't be null in FileStream finalizer


We could if we wanted to split the ctor from the initialization, e.g.

_strategy = new Legacy...(); _strategy.Initialize(argumentsPreviouslyPassedToCtor);

I don't know whether it's worth it.

Co-authored-by: Stephen Toub <stoub@microsoft.com>

jozkee

Otherwise; LGTM, thanks.

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Unix.cs

jozkee · 2021-02-23T17:10:12Z

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Unix.cs

+            return SafeFileHandle.Open(path!, openFlags, (int)OpenPermissions);
+        }
+
+        internal static bool GetDefaultIsAsync(SafeFileHandle handle, bool defaultIsAsync) => handle.IsAsync ?? defaultIsAsync;


Just curious; do you know when DefaultIsAsync can be true? I couldn't find a declaration other than this one:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

Line 17 in a98fe6c

private const bool DefaultIsAsync = false;

You are right, for some reason the const is always false. I wonder why someone has introduced such a const in the first place?

src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Unix.cs

src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.Windows.cs

jozkee · 2021-02-23T18:06:11Z

src/libraries/System.Private.CoreLib/src/System/IO/LegacyFileStreamStrategy.cs

+
+        internal override bool IsClosed => _fileHandle.IsClosed;
+
+        private static bool IsIoRelatedException(Exception e) =>


Could this be moved to the helper class?

Is it OK if I do this in the next PR? In the next PR I would like to introduce two new strategies for Windows and there I will have to move all helper logic to helper type to reduce the code duplication as much as possible.

Co-authored-by: David Cantú <dacantu@microsoft.com>

adamsitnik · 2021-02-24T07:29:01Z

@carlossanlop @jozkee @stephentoub big thanks for all the reviews!

carlossanlop

Thanks for making this change!

adamsitnik added 30 commits January 15, 2021 12:31

rename FileStream -> FileStreamImpl FILES

c0ded00

rename FileStream -> FileStreamImpl CLASS, make it internal and sealed

0fe40d1

restore orgiginal FileStream

e352313

ctors

550e63f

introduce new abstract class to have Lock, Unlock and Handle methods …

62d863c

…to call, implement these methods

implement FlushAsync

14d2aa0

add all virtual methods to the base abstrac class (and rename it)

796d7a3

implement Read(byte[] buffer, int offset, int count)

2108bd6

ReadAsync(byte[] buffer, int offset, int count, CancellationToken can…

3cc6d07

…cellationToken)

ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = …

0a202c0

…default)

Write(byte[] buffer, int offset, int count)

c45e7eb

Write(ReadOnlySpan<byte> buffer)

cedc13a

WriteAsync(byte[] buffer, int offset, int count, CancellationToken ca…

4b3d048

…ncellationToken)

WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellatio…

bafc099

…nToken = default)

Flush() and Flush(bool flushToDisk)

91513ed

CanRead and CanWrite

ecd62f0

input validation should be always performed in the public type

1faf34a

SetLength(long value)

fe308fe

SafeFileHandle, Name, IsAsync and Length

082d630

Position

cb6adc3

IsClosed

2feeefa

ReadByte()

1301318

WriteByte(byte value)

7f15975

finalizer

6089931

BeginRead(byte[] buffer, int offset, int count, AsyncCallback? callba…

d26deb4

…ck, object? state)

BeginWrite(byte[] buffer, int offset, int count, AsyncCallback? callb…

0a9fd33

…ack, object? state)

EndRead and EndWrite

f681b6f

FileStream(SafeFileHandle handle, FileAccess access, int bufferSize)

f880335

fix compilation errors

fceeb0d

remove unused fields and methods

80fdf68

adamsitnik requested review from carlossanlop and jozkee February 5, 2021 17:53

Merge remote-tracking branch 'upstream/master' into fileStream1stStep

ba34492

carlossanlop reviewed Feb 17, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamStrategyBase.cs Outdated Show resolved Hide resolved

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs Outdated Show resolved Hide resolved

adamsitnik mentioned this pull request Feb 18, 2021

[Discussion] Using BufferedStream adamsitnik/runtime#4

Closed

adamsitnik added 4 commits February 22, 2021 19:15

don't separate Unix and Windows strategies, introduce LegacyFileStrea…

a52f5a8

…mStrategy

address code review comment and fix last TODO

3a3b3c9

fix Unix build 1/n

b41456a

update comment

41c3263

stephentoub reviewed Feb 23, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 23, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 23, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStreamHelpers.Windows.cs Outdated Show resolved Hide resolved

stephentoub reviewed Feb 23, 2021

View reviewed changes

stephentoub approved these changes Feb 23, 2021

View reviewed changes

adamsitnik and others added 2 commits February 23, 2021 19:51

Update src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

a88f1a5

Co-authored-by: Stephen Toub <stoub@microsoft.com>

apply code review suggestions

5ad9cbb

jozkee approved these changes Feb 23, 2021

View reviewed changes

Apply suggestions from code review

835fa17

Co-authored-by: David Cantú <dacantu@microsoft.com>

adamsitnik merged commit 87a7159 into dotnet:master Feb 24, 2021

carlossanlop reviewed Feb 24, 2021

View reviewed changes

DrewScoggins mentioned this pull request Mar 4, 2021

[Perf] Regression in Perf_FileStream #49156

Closed

thesn10 mentioned this pull request Mar 4, 2021

System.IO.Pipelines support [POC] [WIP] tusdotnet/tusdotnet#123

Merged

ghost locked as resolved and limited conversation to collaborators Mar 26, 2021

adamsitnik deleted the fileStream1stStep branch July 2, 2021 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce FileStreamStrategy as a first step of FileStream rewrite #47128

Introduce FileStreamStrategy as a first step of FileStream rewrite #47128

adamsitnik commented Jan 18, 2021 •

edited

Loading

adamsitnik commented Feb 8, 2021

carlossanlop left a comment

stephentoub commented Feb 17, 2021 •

edited

Loading

adamsitnik commented Feb 22, 2021

stephentoub Feb 23, 2021 •

edited

Loading

adamsitnik Feb 23, 2021

stephentoub Feb 24, 2021

stephentoub Feb 23, 2021

adamsitnik Feb 23, 2021

stephentoub Feb 23, 2021

jozkee left a comment

jozkee Feb 23, 2021

adamsitnik Feb 24, 2021

jozkee Feb 23, 2021

adamsitnik Feb 24, 2021

adamsitnik commented Feb 24, 2021

carlossanlop left a comment


		internal override bool IsClosed => _fileHandle.IsClosed;

		private static bool IsIoRelatedException(Exception e) =>

Introduce FileStreamStrategy as a first step of FileStream rewrite #47128

Introduce FileStreamStrategy as a first step of FileStream rewrite #47128

Conversation

adamsitnik commented Jan 18, 2021 • edited Loading

adamsitnik commented Feb 8, 2021

carlossanlop left a comment

Choose a reason for hiding this comment

stephentoub commented Feb 17, 2021 • edited Loading

adamsitnik commented Feb 22, 2021

stephentoub Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik commented Feb 24, 2021

carlossanlop left a comment

Choose a reason for hiding this comment

adamsitnik commented Jan 18, 2021 •

edited

Loading

stephentoub commented Feb 17, 2021 •

edited

Loading

stephentoub Feb 23, 2021 •

edited

Loading